Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor Account related components with function component, react-query, and ts #71

Merged
merged 15 commits into from
Jun 1, 2024
Merged

Refactor Account related components with function component, react-query, and ts #71

merged 15 commits into from
Jun 1, 2024

Conversation

yumincho
Copy link
Contributor

@yumincho yumincho commented May 16, 2024 •
edited
Loading

Description

지난 회의(24/05/15)에서 논의한 대로 react-query를 도입합니다. 자세한 내용은 노션에서 확인하실 수 있습니다.

그리고 예시로 account page(@/components/sections/account)에 해당하는 컴포넌트들을 리팩토링 해두었습니다. 작업 사항은 아래와 같습니다.

  1. function component로 변경
  2. react-query 적용
  3. TS 도입 (section 및 page)

Notes

  • Complete the integration of TypeScript with Redux #72 머지 후로 rebase 하면서 기존에 <App/>에 들어가 있던 QueryClientProvider를 index.tsx로 옮겼습니다.
  • account page에 최초로 들어가면 useSessionInfo 불러오느라 깜빡이는 문제가 있는데, main page + login 로직에서 react-query 쓰도록 리팩토링하면 해결될 것으로 보입니다.

React-query Setup Checklist

  • @tanstack/react-query 추가
  • QueryClientProvider로 App 컴포넌트 감싸기 index.tsx에 추가
  • ReactQueryDevtools 추가
  • 예시 코드 작성 (AccountPage에서 사용되는 account.ts를 예시로 작성하였습니다.)
  • AccountPage 내 컴포넌트 및 AccountPage 리팩토링

Todo

@yumincho yumincho self-assigned this May 16, 2024
@codecov Codecov
Copy link

codecov bot commented May 16, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 65.68627% with 35 lines in your changes are missing coverage. Please review.

Project coverage is 5.93%. Comparing base (de39597) to head (aea8afd).

Files Patch % Lines
...sections/account/FavoriteDepartmentsSubSection.tsx 61.29% 12 Missing ⚠️
src/pages/AccountPage.tsx 0.00% 12 Missing ⚠️
src/queries/account.ts 70.00% 6 Missing ⚠️
src/index.tsx 0.00% 3 Missing ⚠️
...onents/sections/account/AcademicInfoSubSection.tsx 93.33% 1 Missing ⚠️
...c/components/sections/account/MyInfoSubSection.tsx 91.66% 1 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##           migration     #71      +/-   ##
============================================
+ Coverage       4.46%   5.93%   +1.46%     
============================================
  Files            213     214       +1     
  Lines           6558    6574      +16     
  Branches        1633    1742     +109     
============================================
+ Hits             293     390      +97     
+ Misses          6169    6023     -146     
- Partials          96     161      +65

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yumincho yumincho marked this pull request as ready for review May 16, 2024 11:42
@yumincho
Copy link
Contributor Author

yumincho commented May 16, 2024 •
edited
Loading

763fdfb 커밋이 문제였네요. React를 import해주던 line 102를 지우면 안 되는 거였습니다. 이 PR에서 수정해도 괜찮을까요? vscode 껐다 켜니까 문제가 사라졌어요 🤔

image

@sboh1214 sboh1214 force-pushed the migration@setup-react-query branch from b2e5f9a to decf1af Compare May 16, 2024 12:24
@yumincho yumincho marked this pull request as draft May 16, 2024 12:41
@yumincho yumincho marked this pull request as ready for review May 16, 2024 12:54
@yumincho yumincho added the migrate migrate from JS CC to TS FC label May 16, 2024
@yumincho yumincho added this to the TypeScript Migration milestone May 16, 2024
@yumincho yumincho changed the title Set up react-query Refactor Account related components with function component, react-query, and ts May 16, 2024
@sboh1214 sboh1214 force-pushed the migration@setup-react-query branch from 01c2260 to 84b8e08 Compare May 16, 2024 14:51
sboh1214
sboh1214 requested changes May 19, 2024
View reviewed changes
Copy link
Contributor

@sboh1214 sboh1214 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/components/sections/account/__tests__/AcademicInfoSubSection.test.tsx 파일 참조하시면 단순 렌더링 테스트 뿐만 아니라, 컴포넌트에 학번과 이메일이 잘 표출되는지 테스트하고 있습니다.
확인하신 후 이런 식의 테스트를 앞으로 작성할지 동방에서 이야기 나누어보면 좋을 것 같습니다.

src/queries/account.ts Outdated Show resolved Hide resolved
src/queries/account.ts Show resolved Hide resolved
@jooyeongmee
Copy link
Contributor

migration 브랜치와 비교해봤을 때 마이페이지에서 설정 부분 텍스트가 로딩 시 사라지는 것 같습니다.
기존 브랜치: 로딩 시 설정 텍스트가 나옴

2024-05-28.00.35.41.mov

현재 브랜치: 로딩 시 설정 텍스트 안 나옴

2024-05-28.00.36.52.mov

그리고 개인적인 의견인데 화면녹화 해보니 설정 부분 데이터 나오는 과정이 상당히 오래 걸리는데(약 15초) 혹시 로딩 시 로딩뷰(?)를 넣는 건 어떨까요?

@yumincho
Copy link
Contributor Author

@jooyeongmee 지적 감사합니다. 수정했어요! bc0afd0

Screen.Recording.2024-05-30.at.14.28.39.mov

@yumincho yumincho force-pushed the migration@setup-react-query branch from bc0afd0 to aea8afd Compare May 30, 2024 07:54
@sboh1214 sboh1214 self-requested a review June 1, 2024 08:26
sboh1214
sboh1214 approved these changes Jun 1, 2024
View reviewed changes
Copy link
Contributor

@sboh1214 sboh1214 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NASA LGTM

@sboh1214 sboh1214 merged commit 6076b84 into migration Jun 1, 2024
5 checks passed
@sboh1214 sboh1214 deleted the migration@setup-react-query branch June 1, 2024 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jooyeongmee jooyeongmee jooyeongmee approved these changes

@sboh1214 sboh1214 sboh1214 approved these changes

@UrWrstNightmare UrWrstNightmare Awaiting requested review from UrWrstNightmare

Assignees

@yumincho yumincho

Labels
migrate migrate from JS CC to TS FC
Projects
None yet
Milestone
Refactoring & Migration
Development

Successfully merging this pull request may close these issues.
3 participants
@yumincho @jooyeongmee @sboh1214